Skip to content

feat: Instantiation payload support for INetworkPrefabInstanceHandler #3430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 40 commits into
base: develop-2.0.0
Choose a base branch
from

Conversation

Extrys
Copy link

@Extrys Extrys commented Apr 27, 2025

Solves Issue #3421

Related to the discussions in Pull Request #3419 and Issue #3421 (follow-up proposal based on new approach).

This PR introduces support for sending custom instantiation payloads through INetworkPrefabInstanceHandlerWithData to receive metadata before calling Instantiate().

The feature is fully optional, backward-compatible, and requires no changes to existing user workflows.

Changelog

  • Added INetworkPrefabInstanceHandlerWithData, a variant of INetworkPrefabInstanceHandler that supports synchronizing custom data prior to the Instantiate() method call.

Testing and Documentation

  • New tests have been added.
  • Inline code documentation provided; INetworkPrefabInstanceHandler.Instantiate() summary updated to mention INetworkPrefabInstanceHandlerWithData
  • Further external documentation is recommended for INetworkPrefabInstanceHandlerWithData.

Deprecated API

  • None.

Backport

  • Can be backported to v1.x. with a cherry pick if merged

Implementation Example

public class MyPrefabInstanceHandler : INetworkPrefabInstanceHandlerWithData
{
    GameObject[] prefabs;
    public int customSpawnID = 0;

	//NEW
    void INetworkPrefabInstanceHandlerWithData.OnSynchronizeInstantiationData<T>(ref BufferSerializer<T> serializer)
        => serializer.SerializeValue(ref customSpawnID);

    public NetworkObject Instantiate(ulong clientId, Vector3 position, Quaternion rotation)
        => Object.Instantiate(prefabs[customSpawnID], position, rotation).GetComponent<NetworkObject>();

    public void Destroy(NetworkObject networkObject)
        => Object.Destroy(networkObject.gameObject);
}

Spawning flow:

MyPrefabInstanceHandler prefabInstanceHandler = new MyPrefabInstanceHandler();
GameObject basePrefab;

void RegisterHandler() => NetworkManager.Singleton.PrefabHandler.AddHandler(basePrefab, prefabInstanceHandler);
void UnregisterHandler() => NetworkManager.Singleton.PrefabHandler.RemoveHandler(basePrefab);

public void Spawn(int id)
{
    prefabInstanceHandler.customSpawnID = id; //Plot twist: simply modify the handler's data
    NetworkManager.Singleton.SpawnManager.InstantiateAndSpawn(basePrefab.GetComponent<NetworkObject>());
}

Important

When spawning, you must update the handler's data before calling Spawn() or InstantiateAndSpawn().
The data set in the handler will be serialized automatically prior the instantiation process.

Highlights

  • Optional and non-breaking
  • Intuitive to configure and resilient to errors
  • Fully aligns with NGO patterns (Serialize/Deserialize symmetry)
  • Late-join and scene object support
  • No public API modifications

@Extrys Extrys requested a review from a team as a code owner April 27, 2025 02:26
@Extrys Extrys changed the title feat: Network Object Instantiation Payload feat: Instantiation payload support for INetworkPrefabInstanceHandler Apr 27, 2025
@Extrys
Copy link
Author

Extrys commented Apr 27, 2025

I just posted a video demonstrating how the system works:

  • * is a symbol set on objects spawned through PrefabHandlers.
  • & is a symbol added to indicate the deterministic ID of an object.

In the video, I spawn A1 and A2, which are instances of the same prefab, displaying the letter A.
Each button spawns the prefab with a different number, and this number is sent via the instantiation payload.
No RPCs or NetworkVariables are needed.

The B object is registered with a regular PrefabHandler (no custom interface implemented here, just for basic testing).

D, E, and F are instantiated directly via Unity's regular Instantiate method.
Each of these sets its deterministic ID manually and registers itself into a local dictionary, indexed by that ID.

2025-04-27.21-45-44.mp4

By implementing a custom INetworkPrefabInstanceHandler together with INetworkInstantiationPayloadSynchronizer,
I simply retrieve the ID from the payload and use it to link the correct instance deterministically.

Here is the core implementation:

public class TestHandlerDeterministicLink : INetworkPrefabInstanceHandler, INetworkInstantiationPayloadSynchronizer
{
	public Dictionary<int, DeterministicIDHolder> deterministicSpawns = new Dictionary<int, DeterministicIDHolder>();

	public int customSpawnID = 0;

	void INetworkInstantiationPayloadSynchronizer.OnSynchronize<T>(ref BufferSerializer<T> serializer) => serializer.SerializeValue(ref customSpawnID);

	public NetworkObject Instantiate(ulong clientId, Vector3 position, Quaternion rotation)
	{
		var obj = deterministicSpawns[customSpawnID];
		TMP_Text text = obj.GetComponent<TMP_Text>();
		text.SetText(text.text + "*");
		return obj.GetComponent<NetworkObject>();
	}

	public void Destroy(NetworkObject networkObject) => GameObject.Destroy(networkObject.gameObject);

	int nextDeterministicId = 0;

	public void InstantiateLocally(GameObject linkablePrefab)
	{
		var spawned = GameObject.Instantiate(linkablePrefab);
		spawned.transform.position = UnityEngine.Random.insideUnitCircle * 0.01f;
		var text = spawned.GetComponent<TMP_Text>();
		text.SetText(nextDeterministicId.ToString() + "&" + text.text);
		var deterministicIdHolder = spawned.GetComponent<DeterministicIDHolder>();
		deterministicSpawns[nextDeterministicId] = deterministicIdHolder;
		deterministicIdHolder.SetID(nextDeterministicId);
		nextDeterministicId++;
	}
}

Warning

While this system enables advanced workflows,
it is important to note that developers are responsible for ensuring that the linked instances are compatible.
This flexibility is intentional to support a variety of custom deterministic linking strategies.

@victorLanga17
Copy link

This would actually save us a lot of trouble.

Right now when after we spawn stuff we have to run like two or three RPCs just to finish setting up objects properly.
If we could send a bit of info during the spawn itself I think we could solve a couple of problems easier.

I wish you luck on get it merged on Unity 6.1, it would be super useful for our current project.

@Extrys
Copy link
Author

Extrys commented Apr 28, 2025

This would actually save us a lot of trouble.

Right now when after we spawn stuff we have to run like two or three RPCs just to finish setting up objects properly. If we could send a bit of info during the spawn itself I think we could solve a couple of problems easier.

I wish you luck on get it merged on Unity 6.1, it would be super useful for our current project.

Sure! I'm just waiting for reviewers to be assigned to this PR.
In the worst case, you can always use my fork, which I will keep updated for my use cases only, so sometimes it might not be fully up to date.

@Extrys Extrys marked this pull request as draft April 29, 2025 04:51
@Extrys
Copy link
Author

Extrys commented Apr 29, 2025

I just got more feedback on the Issue #3421

I'm thinking about a way to have the prefab handler "write" the payload right before the spawn happens.
The idea is that this write just stores the data locally in the instance that directly calls CreateObjectMessage, and then the actual message would consume that cached external variable right before sending.

In this approach, I would try to move most of the logic into CreateObjectMessage, removing it from the object data.
Although I feel there would still need to be a way to link the payload to the generated instances to make things work correctly for late joiners and similar cases.

This would avoid all the newly added generics and any potential object boxing.

I'm converting this PR into a draft to keep modifying the implementation and will get back to comment once it's ready for feedback again.

@Extrys
Copy link
Author

Extrys commented Apr 29, 2025

[Sorry bad writting i might edit this text later]

I did requested changes by @EmandM into the PR, currently im reusing the same buffer serializer from the object serialization.
Now the changeset is much smaller and easier to check and review, i would like you to give me feedback on that

Also i could make the new interface to not have the OnSynchronize, and having instead Serialize and Deserialize methods, but that would make the usage not as comfortable.

Copy link
Collaborator

@EmandM EmandM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic next step! The video was super helpful to understand what the intention was. Is there any chance you have a more minimalistic example of this feature that doesn't require linking the items together later. The added complexity of having separate objects that are linked implies that this feature is doing the linking. My understanding is simply that this feature enables changing the object that is instantiated as part of the instantiation flow.

A few notes on the code:

Out of interest, is there a reason you chose to implement the custom data passing only on scene objects? We'd prefer a solution that works everywhere where the prefab handler can work. Again, the symmetry in the approach is important. If you can do something with prefab handlers in one area, that feature should work in all areas.

It would also be fantastic if you can add a test showing how this feature is used.

/// Interface for synchronizing custom instantiation payloads during NetworkObject spawning.
/// Used alongside <see cref="INetworkPrefabInstanceHandler"/> to extend instantiation behavior.
/// </summary>
public interface INetworkInstantiationPayloadSynchronizer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name could be more descriptive. How about something like INetworkPrefabInstantiationHandler?

Copy link
Author

@Extrys Extrys Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this interface doesn't handle instantiation itself, that’s entirely the role of INetworkPrefabInstanceHandler.

The purpose of INetworkInstantiationPayloadSynchronizer is strictly to serialize and deserialize the instantiation payload before Instantiate() is called. That’s why I went with a name that emphasizes its function in data synchronization, rather than suggesting it’s involved in the instantiation logic directly.

If we go with the OnPreInstantiate or OnBeforeInstantiation naming you suggested, perhaps something like INetworkPrefabPreInstantiationHandler or INetworkPrefabBeforeInstantiationHandler would better reflect the purpose. I’m happy to update the name as long as it clearly communicates what the interface does.

I used the term payload since it directly refers to the custom data being passed along with the spawn message, which is exactly what this interface handles

I'll explore a few alternative naming options in tomorrow commit to see if any feel like a better fit.

Copy link
Collaborator

@EmandM EmandM Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with this note, I was also thinking it might be nice if this new interface extends from the base interface.

So rather than needing

class MyHandler : INetworkPrefabInstanceHandler, INetworkInstantiationPayloadSynchronizer

It could instead be used as

class MyHandler : INetworkPrefabPayloadHandler

or something of that type. Keeps it clearer for developers to implement and simpler to use.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty good idea, i like how it looks simpler now, something like INetworkPefabInstanceHandlerWithData would be similar to the old namings of the job system, like IJobComponent IJobWithECB and similar.

INetworkPrefabPayloadHandler looks good to me but I feel INetworkPefabInstanceHandlerWithData its even more descriptive for developers who will use it.

What do you think, could we name it INetworkPefabInstanceHandlerWithData ?
Im sure we can find a better naming later for the second interface it wraps.

/// allowing you to cache or prepare information needed during instantiation.
/// </summary>
/// <param name="serializer">The buffer serializer used to read or write custom instantiation data.</param>
void OnSynchronize<T>(ref BufferSerializer<T> serializer) where T : IReaderWriter;
Copy link
Collaborator

@EmandM EmandM Apr 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid confusion with NetworkBehaviours, could we rename this to something more descriptive like OnInstantiation() or OnBeforeInstantiation()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! As long as the name clearly reflects that this method is solely responsible for synchronizing data prior to instantiation, I’m happy to update it.

OnBeforeInstantiation works well for that, as long as we keep in mind that it is actually serializing and deserializing data

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might rename the interface to something like:
INetworkPrefabPreInstantianceDataSerializer INetworkPrefabPreInstantianceSynchronizer INetworkPrefabPreInstantiantiateHandler
And the method could be:
OnPreInstanceSerialization, SynchronizePreInstance, PreInstanceSynchronization, HandlePreInstantiateData

This way, the naming makes it clearer that its about the data flow before instantiation, not the instantiation itself, and it also avoids confusion with NetworkBehaviour.OnSynchronize()

Let me know if any of these seem closer to NGO’s naming conventions, or if you would prefer a shorter variation

@Extrys
Copy link
Author

Extrys commented Apr 29, 2025

This is a fantastic next step! The video was super helpful to understand what the intention was. Is there any chance you have a more minimalistic example of this feature that doesn't require linking the items together later. The added complexity of having separate objects that are linked implies that this feature is doing the linking. My understanding is simply that this feature enables changing the object that is instantiated as part of the instantiation flow.

A few notes on the code:

Out of interest, is there a reason you chose to implement the custom data passing only on scene objects? We'd prefer a solution that works everywhere where the prefab handler can work. Again, the symmetry in the approach is important. If you can do something with prefab handlers in one area, that feature should work in all areas.

It would also be fantastic if you can add a test showing how this feature is used.

Regarding this, I've tested it, and in the video only one object is an in-scene placed object. The rest are dynamically instantiated through the prefab instance handler, not just scene objects. Maybe I misunderstood what you meant?

I’ll work on a simpler example, though to be honest, the linking case is the most valuable use case I’ve found so far, its actually what motivated this feature in the first place.

Right now I dont have many alternative examples because most of the benefit comes from enabling that exact flow: having objects pre-created and deterministically selected or connected based on payload metadata. Of course, it also opens the door to more advanced use cases, like sending special setup/configuration data before instantiation (For example in the video that sets up A to be configured with a text that says the id to the next), but those are things I imagine users will find creative uses for once the mechanism is available.

In a way, I don’t yet fully know the limits of the feature, I just know it unlocks workflows that weren’t previously possible.

About the other changes, I will answer these and also come with some changes you might like tomorrow.

@Extrys
Copy link
Author

Extrys commented Apr 30, 2025

Although the struct is named SceneObject, it’s already used across the codebase to represent any instance of NetworkObject, not just those previously placed in the scene.
Even dynamically instantiated objects are serialized using this structure.
So this change applies universally to all uses of INetworkPrefabInstanceHandler, not only in-scene-object cases.


With that in mind, I’ve prepared a visual comparison to clarify why the deserialization buffer for the instantiation payload currently lives inside SceneObject.Deserialize
image

As shown above, the HasInstantiationPayload flag is handled symmetrically in both Serialize() and Deserialize(). Keeping the payload deserialization inline within SceneObject.Deserialize ensures that all object-related synchronization logic remains co-located, which helps maintain readability and traceability when following the execution flow.

Moving the deserialization logic to AddSceneObject, while possible, would separate serialization and deserialization across different layers.

That said, since AddSceneObject already deals with other responsibilities like calling SynchronizeNetworkBehaviours, moving the deserialization logic there wouldn’t be unreasonable either, especially if buffer reuse is preferred.

Here's a visual representation of how that could look if moved into AddSceneObject:
image

I’m happy to move the deserialization logic if preferred, just wanted to show why it currently sits in SceneObject.Deserialize() to keep things cohesive and close to where the payload is originally serialized.

Let me know what direction you'd like to take, and I’ll gladly adapt the implementation accordingly.


About the naming
I'm open to renaming the interface and method to better align with NGO conventions. Below are a five of options I’ve considered:

/// Option A  - Short concise and easy to undesrtand
INetworkPrefabInstanceDataHandler
   HandleInstanceData()

// Option B   
INetworkPrefabPreInstanceDataHandler
   HandlePreInstanceData()
   
// Option C   - This is also pretty descriptive
INetworkPrefabInstanceSynchronizer
   SynchronizeInstanceData()
   
// Option D
INetworkPrefabInstanceDataSerializer 
   SerializeInstanceData()

// Option E - For me, this is the most descriptive
INetworkPrefabInstantiancePayloadHandler
   HandleInstancePayload()

Happy to hear your thoughts or preferences here!

@EmandM
Copy link
Collaborator

EmandM commented Apr 30, 2025

I had another pass today. Definitely agree with what you've said about SceneObject! Unfortunately the reason SceneObject.Serialize and SceneObject.Deserialize are not perfect mirrors is due to the deferred messaging system for distributed authority, so we do need to follow the SynchronizeNetworkBehaviours flow. That does mean deserializing inside the AddSceneObject function, just before the call to CreateLocalNetworkObject.

I also took a bit more time with the example. I absolutely see what you're doing there. Thank you for the detailed explanations.

It'll be best if the function naming we go with follows the On<event> naming convention. Also how do you feel about the idea of having the new interface extend from INetworkPrefabInstanceHandler?

Mixing and matching from your naming options, what do you think of something like these two options?

/// Option A  
INetworkPrefabInstanceWithDataHandler
   OnPreInstantiate()
   
// Option B
INetworkPrefabWithSynchronizeHandler
   OnPrefabSynchronize()

@Extrys
Copy link
Author

Extrys commented Apr 30, 2025

I had another pass today. Definitely agree with what you've said about SceneObject! Unfortunately the reason SceneObject.Serialize and SceneObject.Deserialize are not perfect mirrors is due to the deferred messaging system for distributed authority, so we do need to follow the SynchronizeNetworkBehaviours flow. That does mean deserializing inside the AddSceneObject function, just before the call to CreateLocalNetworkObject.

No problem, I’ll make that change in a few minutes!


Also how do you feel about the idea of having the new interface extend from INetworkPrefabInstanceHandler?

Its a perfect idea, making it easier to use for developers. The next commit will include that.


Mixing and matching from your naming options, what do you think of something like these two options?

Since it’s still an INetworkPrefabInstanceHandler, just extended with support for instantiation-time data, I think INetworkPrefabInstanceHandlerWithData makes the most sense.

In contrast, INetworkPrefabInstanceWithDataHandler might suggest that the "instance" has a data handler, which doesn’t quite match the intended semantics.

Would this option work for you?

public interface INetworkPrefabInstanceHandlerWithData : INetworkPrefabInstanceHandler
{
   void OnSynchronizeInstantiationData<T>(ref BufferSerializer<T> serializer) where T : IReadWrite
}

The method name OnSynchronizeInstantiationData clearly indicates that it’s used to synchronize data for the instantiation process, implying that this happens before the object is actually instantiated.

Let me know what you think. I’ll go ahead and push a commit with these changes in the meantime and await your feedback. 😄

Extrys added 2 commits May 1, 2025 01:32
1) Moved the payload deserialization into the AddSceneObject, for deferred instantiation compatibility
2) Changed the new interface to be a direct single extended implementation, instead a complement of the handler
3) Some renames to better match what the feature does for developers
@Extrys
Copy link
Author

Extrys commented Apr 30, 2025

All requested changes have been implemented.

  • Moved the payload deserialization into the AddSceneObject, for deferred instantiation compatibility
  • Changed the new interface to be a direct single extended implementation, instead a complement of the handler
  • Some renames to better match what the feature does for developers

This is the same example shown earlier, but simplified and updated to reflect the new interface and naming conventions:

public class TestHandlerDeterministicLink : INetworkPrefabInstanceHandlerWithData
{
    Dictionary<int, DeterministicIDHolder> deterministicSpawns = new Dictionary<int, DeterministicIDHolder>();

    int nextDeterministicId = 0;

    int customSpawnID = 0;

    public void OnSynchronizeInstantiationData<T>(ref BufferSerializer<T> serializer) where T : IReaderWriter
    {
        serializer.SerializeValue(ref customSpawnID);
    }

    public NetworkObject Instantiate(ulong clientId, Vector3 position, Quaternion rotation)
    {
        return deterministicSpawns[customSpawnID].GetComponent<NetworkObject>();
    }

    public void Destroy(NetworkObject networkObject) => GameObject.Destroy(networkObject.gameObject);

    public void DoSpawn(GameObject linkablePrefab)
    {
        var deterministicIdHolder = GameObject.Instantiate(linkablePrefab).GetComponent<DeterministicIDHolder>();
        deterministicSpawns[nextDeterministicId] = deterministicIdHolder;
        deterministicIdHolder.SetID(nextDeterministicId);
        nextDeterministicId++;
    }
}

Marking the pull request as ready for review ✅
Let me know if anything else is needed!

@Extrys Extrys marked this pull request as draft May 8, 2025 11:29
@Extrys
Copy link
Author

Extrys commented May 8, 2025

Hey @EmandM, marking this as draft again. Feel free to skip the earlier messages. This one reflects the most relevant update.
I just uncovered a new edge case in real production use and have come up with a cleaner and more robust approach that I’m confident you’ll find much better.

the edge case in question:
When a networked object changes in a way that should affect how it is instantiated for late joiners (e.g. a weapon dropped by an enemy becomes a world item), the original instantiation metadata can become outdated. Late joiners would still use the old metadata, leading to invalid reconstruction (e.g. re-spawning the enemy that dropped the weapon).


To solve this cleanly, I'm moving toward a stateless and fully explicit model.
Instantiation metadata will be set externally on a per-instance basis.

For clarity: whenever I refer to "the handler" in this context, I'm specifically talking about INetworkPrefabInstanceHandlerWithData<T>, not Unity’s default INetworkPrefabInstanceHandler.

This involves:

  • Enforcing a strongly-typed contract on the handler interface for serialization.
  • Exposing a method on NetworkObject to explicitly inject metadata, ensuring deterministic behavior and full context control.

A typical flow would look like this:

networkObject.SetInstantiationData(instantiationData); 
//Throws an exception if no handler is associated with compatible data

networkObject.Spawn();
// If data was injected, it will be serialized and used during instantiation.
// If not, an exception is thrown to prevent undefined behavior.

Internally, the system can safely check if the injected metadata matches the handler’s expected type and apply it if compatible.

And the handler interface would become even simpler:

public class MySpawnerWithData : INetworkPrefabInstanceHandlerWithData<T> where T : struct, INetworkSerializable
{
    public NetworkObject Instantiate(ulong clientId, Vector3 position, Quaternion rotation, T instantiationData)
    {
        // Logic based on instantiationData
    }
}

With this design, the handler is fully stateless and reacts only to the data provided at instantiation.

This avoids bugs from reused or outdated values and gives developers precise control over how each object is spawned. It also supports dynamic workflows where an object's context may evolve after creation, without relying on RPCs or mutable state.

Since instantiation data is now injected explicitly, the spawning logic no longer needs to handle implicit synchronization. The system stays lean and efficient, with no added overhead and unchanged performance.

I’ll finalize the implementation and push the updated version shortly.
Let me know if you see any red flags or have thoughts before I wrap it up.

@EmandM
Copy link
Collaborator

EmandM commented May 8, 2025

Thanks everyone for all the extra use cases! Makes it easier to reason about what is needed.

I think I'm seeing two top level approaches to this type of feature:

  1. Set the data when the authority locally spawns the object, and then the data is fixed on the object on that point for the rest of the game and all network spawns/late join spawns use the configured data

or

  1. Each time any object is synchronized over the network, the associated prefab handler has the opportunity to send custom data on the spawn call

I was understanding that option 2 was the request, but I see here that option 1 might be more what was intended.

Thanks for the patience with this process. Adding new features always take a lot of back and forth and consideration. I've also been sick the last few days, sorry for the delay in responding.

@Extrys
Copy link
Author

Extrys commented May 8, 2025

Yeah, because I initially went with option 2, but real production use quickly exposed issues.
I’ve since shifted to option 1, with one key addition: the ability to explicitly override instantiation data if the object’s context changes (e.g. a dropped weapon).

The idea is to set metadata once per instance and have it persist across all synchronizations, including for late joiners.

Handlers remain fully stateless and act only on the injected data.

Hope that clarifies the approach. Really appreciate your input, and hope you're feeling better soon!

@Extrys
Copy link
Author

Extrys commented May 8, 2025

The full implementation is ready

Handler definition:

public class HandlerWithData : INetworkPrefabInstanceHandlerWithData<MyData>
{
    public NetworkObject Instantiate(ulong ownerClientId, Vector3 position, Quaternion rotation, MyData data)
    {
        //My logic with this instantiationData for instance handling
    }

    public void Destroy(NetworkObject networkObject) => throw new NotImplementedException();
}

Server-side spawn flow:

networkObject.InjectInstantiationData(instantiationData); 
networkObject.Spawn();

Additionally, I improved the integration tests for this feature and added a new one to simulate late joining.
Marking this PR as ready for review again.

@Extrys Extrys marked this pull request as ready for review May 8, 2025 23:22
Extrys added 2 commits May 9, 2025 02:10
Clarifies the purpose and usage of the static data map.
@Extrys
Copy link
Author

Extrys commented May 12, 2025

Gentle ping @EmandM, this PR has been ready and production-tested for 3 days. Let me know if there's anything blocking the review or if I can help clarify any part. The implementation includes late join support and updated integration tests.

@EmandM
Copy link
Collaborator

EmandM commented May 12, 2025

Thank you for the updates! Unfortunately we still have some design and refinement to do on this feature.

If we are going with option 1, we should consider whether it is necessary to have logic on the prefab handler. If the prefab handler is still a core part of this feature, then we need to keep the logic inside the prefab handler. It's important that as much of the feature as possible lives in one place, that way as NGO grows and shifts over time, this feature is easy to maintain and does not break due to future maintainers not understanding the usage paths of the code.

We do not want to expose the intermediate interface to the public (code docs aside, there will be games that build on top of the intermediate interface). As I know a public interface cannot inherit from a private interface, we need to reconsider that approach.

I am also uncomfortable with adding a public function to NetworkObject that has the invisible requirement that this object must have an associated PrefabHandler. That feels unintuitive to me. From past experience, that will become an area where issues will be raised in the future (of the "Can't use the instantiation data outside the prefab handler" type).

We're super interested in this feature and really want to find a way to get it working. We have a few options we're mulling over internally, though we do not have time to take over the development at this time.

@Extrys
Copy link
Author

Extrys commented May 12, 2025

Okay @EmandM, I’ve moved the injection logic fully inside the prefab handler.
The intermediary interface is gone, and NetworkObject no longer has any new public or internal APIs.

This should now fully align with your architectural concerns:

  • Logic is centralized within the PrehabHandler
  • No public exposure of intermediate types
  • No API additions on NetworkObject

With that in place, the part of the implementation that previously didn’t look ideal now results in two possible approaches:

A

NetworkManager.Singleton.PrefabHandler.InjectInstantiationData(networkObject, data);
networkObject.Spawn();

It’s now more aligned with the prefab handler conceptually, but relying on a global call still feels somewhat indirect and tightly coupled to the NetworkManager.

So, in the latest commit 3dea6aa, I extended the handler interface to support a more intuitive usage:

B

// data is already type-constrained here, so invalid types cannot be passed
myHandlerWithData.InjectInstantiationData(instance, data);
instance.Spawn();

//or alternatively, just call:

myHandlerWithData.SpawnWithData(instance, data);

Once you decide my implementation is solid enough, I’ll mark the PR as ready for review again.
I just need your feedback first, as I still have to document the new code and clean up the diffs to ensure the branch is merge-ready.

You mentioned there were some internal options being considered, I’d be interested in hearing more about them, especially if they could help guide this to completion.
I’m continuing to work on this since it’s a key dependency for several internal projects.

Additionally, to be fully honest, having something officially accepted into Unity would mean a lot to me personally. It’s not just about the feature, it would be a meaningful milestone in my career to contribute something that becomes part of the engine. That’s why I’m not giving up.
So please don’t hesitate to keep suggesting changes. I don’t see them as something negative, but as valuable feedback that helps me focus my time on making this solid enough to be officially accepted.

@Extrys Extrys marked this pull request as draft May 12, 2025 22:03
@EmandM
Copy link
Collaborator

EmandM commented May 13, 2025

I think we're moving in the right direction. Unfortunately we have to slow down the review process a bit for now as we're close enough to a good solution that it takes some time to go over each iteration and work through the side effects and consequences of the changes.

Internally we were considering that the api for the INetworkPrefabInstanceHandler.Instantiate method is a bit opinionated and brittle. We were wondering about adding overrides for the PrefabHandler methods that take INetworkPrefabInstanceHandler and introducing a new interface (or abstract class potentially) that takes a struct as an argument for Instantiate. It's actually pretty close to what you have there, simply used more directly and without the HandlerWrapper. My concern with the HandlerWrapper is the same as when we had the OnPreInstantiate() flow - we can't be 100% sure that the payload that is read from ReadInstantiationData is the same payload that is getting passed to Instantiate. We can be mostly sure for now, but not sure in a way that is future proof.

I see what you're doing with the INetworkPrefabInstanceHandlerWithDataExtensions method. I personally really like that way of doing things, but I know from experience that it'll lead to confusion in the future. Best to leave the InjectInstantiationData method on the prefab handler itself I think. I also think the naming potentially works better as SetInstantiationData.

If you're willing to hang on through the (longwinded) feature development process, then we're happy to work with you to get this feature landed under your name. It just may take some time as adding new features is a risky process with many future considerations to keep in mind.

@Extrys
Copy link
Author

Extrys commented May 14, 2025

Thanks for the update and the transparency. I understand all your concerns.

Regarding the extension methods:
Your point makes sense. If leaving InjectInstantiationData in the PrefabHandler is clearer for both users and maintainers, I’m happy to drop the extension and adopt the SetInstantiationData naming, which also sounds much better.

About the payload (stateful wrapper) concern:
I see how that pattern could cause issues in the future if the flow changes before the payload is used. Passing the payload explicitly through the post-deserialization path seems like the safest long-term approach, even if it requires some internal refactoring.

Originally, I avoided modifying the instantiation flow to keep method signatures intact, but I’m now considering a parallel path for payload-based instantiation that preserves existing behavior.

Let me know if you're comfortable with me exploring a pure stateless approach to pass the data. I’ll do my best to minimize code changes and maintain full backwards compatibility, though it may take a bit longer than previous iterations. I want to make sure the design aligns well with all the feedback you’ve shared throughout this PR.

Also, since I’m already using these changes in real production, I may come back with additional edge cases or refinements as they surface. I’ll make sure any such updates stay aligned with the direction we’re defining here.

I really appreciate the opportunity to work on this with you. I’m fully committed to seeing it through, regardless of how long it takes. ❤️

@EmandM
Copy link
Collaborator

EmandM commented May 14, 2025

Feel free to change any internal methods that need changing. Those are completely free to change at any time!

I like the NetworkObject part of this approach. Setting the data internally on the NetworkObject itself makes sense. A local state only approach to getting the data into the Instantiate method works best, where it is definitely the same variable that was decoded from the buffer that is passed into the Instantiate call.

I think it'll work well simply having a branch when the Instantiate method is called. i.e. call INetworkPrefabInstanceHandler.Instantiate if the type is a INetworkPrefabInstanceHandler, otherwise call the new type of prefab handler.

Internally we were considering having a new type of prefab handler that takes a struct as an argument, and that way we have a lot more flexibility with adding features to the prefab (e.g. something like NetworkObject Instantiate(InstantiationData data);). I haven't thought through or fleshed out exactly how that'd work with your suggested feature, it's simply that internally we foresee future issues much like yours where it'd be nice to pass more data into the instantiate call. The current INetworkPrefabInstanceHandler design is very brittle in that way (as you're finding!)

@michalChrobot
Copy link
Collaborator

michalChrobot commented May 15, 2025

I just want to bump a quick comment to this thread that I really like the cooperation here, how this is developing and while I don't have sufficient knowledge to help more with those topics I love the energy in this thread and I'm looking forward to how it develops 👀 (having in mind that it's always complicated process with many bumps on the way)

@Extrys
Copy link
Author

Extrys commented May 16, 2025

I've already implemented the required changes addressing all feedback:

  • The system is now fully stateless
  • I'm using FastBufferWriter to pass the data between deserialization and Instantiate, avoiding the need for generics in method changes

This version aligns closely with the current architecture and feedback. I’d appreciate any thoughts on whether this direction feels more in line with your expectations, especially considering clarity, maintainability, and minimal disruption to existing workflows

Additionally, I’ve prepared a branch extending this change with more ambitious improvements for future-proofing
I kept it separate to keep this PR clean and easy to compare, it’s a PR on my own fork targeting my local branch.
(Ready to be merged into this PR if you like it)

Key improvements in that extended branch: Please, check extended branch PR

  • New interfaces
    • public INetworkPrefabInstanceHandlerSource: Common abstraction for all handler types, exposing a consistent adapter regardless of instantiation data
    • internal INetworkPrefabInstanceHandlerAdapter: Centralizes the instantiation flow under a single contract, removing the need for branching based on handler type

While INetworkPrefabInstanceHandlerAdapter remains internal, its design allows safe exposure for custom user-defined adapters if needed

  • Adapter-based routing

    • All handler types (INetworkPrefabInstanceHandler, INetworkPrefabInstanceHandlerWithData<T>, future variants...) now expose an adapter via INetworkPrefabInstanceHandlerSource
    • Eliminates type checks and separate dictionaries by handler type
    • Unifies all instantiation logic: for example, AddHandler now operate via the unified interfaces
    • The adapter determines whether custom data is needed, removing branching and parallel data paths
    • The system operates entirely through the adapter interface, reducing complexity and maintenance overhead
  • Compatibility adapters
    LegacyHandlerAdapter and PrefabInstanceHandlerWithDataAdapter wrap existing implementations without changing their internal behavior, for backwards compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants